CBST2-02: Make proposer commitment signatures unique to modules #329

jclapis · 2025-07-01T21:29:21Z

This is a large change that modifies the ways proposer commitment signatures are generated. Currently, the requesting module isn't embedded into the data being signed so any authorized module can request the signature of any data - including data that might be used or expected by other modules. This PR precludes that possibility by embedding a new special per-module signing ID into the data being signed.

The signing ID is a 32-byte hex string provided in the Commit Boost configuration's [[modules]] section, per module. Module authors can make it whatever they want, and should publish it within their own documentation so users know which unique value to configure for that module. Since Commit Boost doesn't (and won't) maintain a global registry of such IDs, the onus is on the user to ensure the correct ID is entered for each module. Module authors can then directly bake this into their module code, in their signature validation routines, to confirm that any proposer commitments returned by the signer used the correct ID.

Since different modules will have different signing IDs (a condition enforced by the loader logic), this prevents one module from "forging" a signature destined for another one.

Using a discrete signing ID means module authors can change their module ID at any time (such as with a version number bump or a vendor rename) without invalidating previous signatures. Conversely, if they do want to invalidate previous signatures while retaining the module ID, they can do so by simply changing their signing ID and informing their clients to update with the new configuration accordingly.

This also adds new round-trip integration tests that confirm signing works as expected with the new paradigm and cross-module signatures are no longer possible.

Finally, it starts some documentation on how to request proposer commitments from the signer service, including how to request a signature and verify it. See docs/docs/developing/prop-commit-signing.md.

⚠️ This is a breaking change that will invalidate any existing proposer commitment signatures. Once this is added, clients using the signer module will need to be informed of the breaking change and adapt their signature verification routines with the new logic.

…s. static

…he source" This reverts commit 58c6117.

ltitanb · 2025-07-09T12:24:12Z

config.example.toml

@@ -239,6 +239,8 @@ proxy_dir = "./proxies"
 [[modules]]
 # Unique ID of the module
 id = "DA_COMMIT"
+# Unique hash that the Signer service will combine with the incoming data in signing requests to generate a signature specific to this module
+signing_id = "0x6a33a23ef26a4836979edff86c493a69b26ccf0b4a16491a815a13787657431b"


does this need to be 32 bytes? something shorter like we do for chain id could also work

It doesn't need to be, it's just 32 out of convenience to match with object_root since the tree hash lib already works with it. It just needs to be unique with a very large space to prevent overlaps; like Chain ID is technically supposed to be very large for the same purpose (https://chainlist.org/). I don't have much reason to make it smaller since it's literally just a generate-once-and-put-it-in-your-module-docs value, but if there is a compelling case for it then sure.

ltitanb · 2025-07-09T12:27:33Z

crates/common/src/config/module.rs

@@ -37,6 +38,8 @@ pub struct StaticModuleConfig {
    /// Type of the module
    #[serde(rename = "type")]
    pub kind: ModuleKind,
+    /// Signing ID for the module to use when requesting signatures
+    pub signing_id: Option<B256>,


why make it optional?

also worth creating a type alias for clarity

why make it optional?

So this was a concession I had to make in order to support the other use cases where the signature generation pipeline is used with COMMIT_BOOST_DOMAIN, but not tied to a specific module. For example, proxy key generation will do this. Proxy keys are already stored by module ID and don't have any sense of signing ID, so coupling them to the signing ID as well would either involve storing that as part of their path or adding a lookup from module ID to signing ID to the signer. Either way, any time the signing ID changes (e.g, when previous signatures need to be invalidated), you'd have to regenerate all of the proxy keys. It seemed easier to make this field optional to support the existing calls instead of refactoring all of that.

ltitanb · 2025-07-09T12:28:12Z

crates/common/src/config/constants.rs

+pub const SIGNER_JWT_AUTH_FAIL_LIMIT_DEFAULT: u32 = 3;
+
+/// How long to rate limit the client after auth failures
 pub const SIGNER_JWT_AUTH_FAIL_TIMEOUT_SECONDS_ENV: &str =
    "CB_SIGNER_JWT_AUTH_FAIL_TIMEOUT_SECONDS";
+pub const SIGNER_JWT_AUTH_FAIL_TIMEOUT_SECONDS_DEFAULT: u32 = 5 * 60;


is this supposed to be in this PR or does it need to be rebased?

This was some incidental cleanup of #310 that landed in here after it was merged. There are a few little non-functional changes related to it. For cleanliness I can pull all of those out and make a separate PR for them if you'd like, no problem.

ltitanb · 2025-07-09T12:35:45Z

crates/common/src/config/signer.rs

+    jwt_secrets: &HashMap<ModuleId, String>,
+) -> Result<HashMap<ModuleId, ModuleSigningConfig>> {
+    let mut mod_signing_configs = HashMap::new();
+    if let Some(modules) = &config.modules {


if this is None this function should not be called, we should error if it is, something like

let Some(modules) = &config.modules else { bail!("...") }

Confirmed, fixed in 1a0efec.

ltitanb · 2025-07-09T12:37:33Z

crates/common/src/config/signer.rs

+        let mut seen_jwt_secrets = HashMap::new();
+        let mut seen_signing_ids = HashMap::new();


if we go through modules one by one, then these could just be hashsets

The value (the module_id) is used in the error messages that get thrown during duplication detection so the user knows which module entries are conflicting. We can swap to HashSets, just print the offending duplicated value itself, and let the user find it in the config if that's preferable, but listing the modules directly seems like good UX.

ltitanb · 2025-07-09T14:35:36Z

crates/common/src/types.rs

+#[derive(Default, Debug, TreeHash)]
+pub struct PropCommitSigningInfo {
+    pub data: [u8; 32],
+    pub module_signing_id: [u8; 32],


B256 implements TreeHash

Yeah, I left these as [u8; 32] because the original SigningData above it used those. Happy to convert everything to B256 though, in fact I'd prefer it if that's ok.

ltitanb · 2025-07-09T14:36:30Z

crates/common/src/signature.rs

+    let signing_root = match module_signing_id {
+        Some(id) => compute_signing_root(&types::SigningData {
+            object_root: compute_signing_root(&types::PropCommitSigningInfo {
+                data: msg.tree_hash_root().0,
+                module_signing_id: id.0,
+            }),
+            signing_domain: domain,
+        }),
+        None => compute_signing_root(&types::SigningData {
+            object_root: msg.tree_hash_root().0,
+            signing_domain: domain,
+        }),
+    };


this logic is repeated a couple of times can we refactor to a separate function?

Done in 133447d.

ltitanb · 2025-07-09T14:38:15Z

crates/common/src/signature.rs

@@ -52,14 +48,27 @@ pub fn verify_signed_message<T: TreeHash>(
    pubkey: &BlsPublicKey,
    msg: &T,
    signature: &BlsSignature,
+    module_signing_id: Option<&B256>,


Option here and below can be removed, we only do signatures in the signer module and modules will always need to have the id set after this PR

See the earlier response - the proxy key generator uses it too, and it (currently) doesn't include signing IDs which is why it's an Option in here. Validating the signature in e.g. SignedProxyDelegationBls won't work without it.

ltitanb · 2025-07-09T14:44:37Z

docs/docs/developing/prop-commit-signing.md

+
+## The Signing ID
+
+Your module's signing ID is a 32-byte value that is used as a unique identifier within the signing process. Preconfirmation signatures incorporate this value along with the data being signed as a way to create signatures that are exclusive to your module, so other modules can't maliciously construct signatures that appear to be from your module. Your module must have this ID incorporated into itself ahead of time, and the user must include this same ID within their Commit Boost configuration file section for your module. Commit Boost does not maintain a global registry of signing IDs, so this is a value you should provide to your users in your documentation.


nit: not "preconfirmation signatures" but "commitment signatures", preconfs are just one type of commitment

Yep good catch! Old verbiage from before Drew gave them a formal name. Fixed in ee282da.

ltitanb · 2025-07-09T14:49:05Z

docs/docs/developing/prop-commit-signing.md

+#### `200 OK`
+
+A successful signing request, with the signature provided as a plaintext quoted hex-encoded string, with a `0x` prefix. For example, the response body would look like:
+```
+"0xa43e623f009e615faa3987368f64d6286a4103de70e9a81d82562c50c91eae2d5d6fb9db9fe943aa8ee42fd92d8210c1149f25ed6aa72a557d74a0ed5646fdd0e8255ec58e3e2931695fe913863ba0cdf90d29f651bce0a34169a6f6ce5b3115"
+```
+
+#### `401 Unauthorized`
+
+Your module did not provide a JWT string in the request's authorization header, or the JWT string was not configured in the signer service's configuration file as belonging to your module.
+
+


i wonder if this part is actually needed? we do have an OpenAPI schema after all

That would be lovely, because maintaining it manually is a chore. I'll ping you about it and add the results to this PR once we have them.

ltitanb and others added 30 commits May 13, 2025 17:17

bump version

c68125d

Successful cross-compilation, but runtime has memory allocation issues

d9979a2

Working with OpenSSL static-linked

97ef653

Got dynamic linking working, added a feature flag to toggle dynamic v…

91eefe2

…s. static

Fixed the vendored build arg

de09415

Reintroduced the cargo chef setup

3aee63d

Ported the cross-compilation stuff into PBS

c07c717

Split the dockerfiles into separate builder / image definitions

699b7ec

Added a build guide

7165f12

Refactored the Github release action to use the Docker builder

9438dae

Fixed the Docker image binary filenames

12c020a

Cleaned up the Darwin artifact step

53cafc0

Made the CI workflow and justfile use the same toolchain as the source

58c6117

Revert "Made the CI workflow and justfile use the same toolchain as t…

45e581b

…he source" This reverts commit 58c6117.

Testing removal of OpenSSL vendored option

24a10c5

Updating just in the CI workflow

e36da54

Merge branch 'main' into cross-compile

843b110

Refactored the signer to support host and port config settings

e7c6d19

Updated docs

6117219

Fixing Clippy in CI workflow

c0f591d

Removed obviated CI setup

adbd34a

Minor dedup of RwLock guard acquisition

e3488b3

Added rate limiting for signer clients with repeated JWT auth failures

c3d7ec4

Added Signer config validation

9ddad64

Started unit test setup for the Signer

c62185e

Finished a basic signer module unit test

dc73c62

Added a JWT failure unit test

6c3d967

Added a rate limit test and cleaned up a bit

6464638

Added unique ports to unit tests for parallel execution

0313f18

Cleaned up the build Dockerfile and removed an extra dependency layer

346eea4

jclapis added 13 commits June 12, 2025 16:45

Started migration from JWTS_ENV to the config file

71a7605

Signing requests now uses the module's signing ID

615774e

Finished added signing ID support and a quick test

af6076d

Fixed some example config parameters

cc13a6f

Added a test to ensure modules can't create the same sigs

488547a

Merge branch 'rate-limit-jwt' into prevent-cross-module-sigs

ef3d8f2

Made the jwt_config_file optional

6fd4327

Started working on docs

d9ef82f

Redid implementation with the original JWTS env var

83db727

Started the signer doc

8d08c11

Overhauled the signing_id setup to be directly in the signed struct

64b49f2

Made proposer commitments nested Merkle trees to allow Dirk support

8b65b1e

Added the signer request guide

9967033

jclapis self-assigned this Jul 1, 2025

jclapis added the signer Signer module label Jul 1, 2025

jclapis added 4 commits July 1, 2025 17:38

Added quotes to some HTML

adb1cb8

Added some simple JWT secret info

1c3a07d

Adding a closing tag

daf3147

Merge branch 'main' into prevent-cross-module-sigs

fa36a34

jclapis marked this pull request as ready for review July 8, 2025 05:49

jclapis changed the base branch from main to sigp-audit-fixes July 8, 2025 08:23

ltitanb requested review from ltitanb and ManuelBilbao July 8, 2025 19:47

Added prop commit signature verification helpers for modules to use

d5641df

ltitanb requested changes Jul 9, 2025

View reviewed changes

jclapis added 5 commits July 10, 2025 02:49

Fixed some params in da_commit

5da31bf

Merge branch 'main' into prevent-cross-module-sigs

bfd72c5

Cleaned load_module_signing_configs a bit

1a0efec

Fixed some docs language

ee282da

Refactored into compute_prop_commit_signing_root

133447d

		let mut seen_jwt_secrets = HashMap::new();
		let mut seen_signing_ids = HashMap::new();


		## The Signing ID

		Your module's signing ID is a 32-byte value that is used as a unique identifier within the signing process. Preconfirmation signatures incorporate this value along with the data being signed as a way to create signatures that are exclusive to your module, so other modules can't maliciously construct signatures that appear to be from your module. Your module must have this ID incorporated into itself ahead of time, and the user must include this same ID within their Commit Boost configuration file section for your module. Commit Boost does not maintain a global registry of signing IDs, so this is a value you should provide to your users in your documentation.

CBST2-02: Make proposer commitment signatures unique to modules #329

Are you sure you want to change the base?

CBST2-02: Make proposer commitment signatures unique to modules #329

Uh oh!

Conversation

jclapis commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jclapis Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltitanb Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jclapis commented Jul 1, 2025 •

edited

Loading

jclapis Jul 14, 2025 •

edited

Loading

ltitanb Jul 9, 2025 •

edited

Loading